Fix error with PEP 517 builds when wheel exists (GH #1761) #1745
Conversation
There was a problem hiding this comment.
It would be better to use this for Python 2 compatibility:
setuptools/py31compat.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git i/setuptools/py31compat.py w/setuptools/py31compat.py
index 1a0705ec..e1da7ee2 100644
--- i/setuptools/py31compat.py
+++ w/setuptools/py31compat.py
@@ -17,9 +17,9 @@ class TemporaryDirectory:
errors on deletion.
"""
- def __init__(self):
+ def __init__(self, **kwargs):
self.name = None # Handle mkdtemp raising an exception
- self.name = tempfile.mkdtemp()
+ self.name = tempfile.mkdtemp(**kwargs)
def __enter__(self):
return self.nameSo you can use a context manager, ensuring the temporary directory is removed in case of error.
I would also prefer if the code was simplified to always use a temporary directory, as the performance impact should be minimal (since rename can be used).
A more important concern: what if the resulting wheel already exists? The call to os.rename will fail on non-Unix platforms.
Modified test to check for this particular issue:
setuptools/tests/test_build_meta.py | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git c/setuptools/tests/test_build_meta.py w/setuptools/tests/test_build_meta.py
index 90400afc..88e29ffe 100644
--- c/setuptools/tests/test_build_meta.py
+++ w/setuptools/tests/test_build_meta.py
@@ -157,7 +157,6 @@ def test_build_wheel(self, build_backend):
assert os.path.isfile(os.path.join(dist_dir, wheel_name))
- @pytest.mark.xfail(reason="Known error, see GH #1671")
def test_build_wheel_with_existing_wheel_file_present(self, tmpdir_cwd):
# Building a wheel should still succeed if there's already a wheel
# in the wheel directory
@@ -195,6 +194,12 @@ def test_build_wheel_with_existing_wheel_file_present(self, tmpdir_cwd):
assert os.path.isfile(os.path.join(dist_dir, wheel_one))
assert wheel_one != wheel_two
+ # and if rebuilding the same wheel?
+ open(os.path.join(dist_dir, wheel_two), 'w').close()
+ wheel_three = self.get_build_backend().build_wheel(dist_dir)
+ assert wheel_three == wheel_two
+ assert os.path.getsize(os.path.join(dist_dir, wheel_three)) > 0
+
def test_build_sdist(self, build_backend):
dist_dir = os.path.abspath('pip-sdist')
os.makedirs(dist_dir)And fix in build_meta:
setuptools/build_meta.py | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git c/setuptools/build_meta.py w/setuptools/build_meta.py
index 47cbcbf6..3472bf8a 100644
--- c/setuptools/build_meta.py
+++ w/setuptools/build_meta.py
@@ -37,6 +37,7 @@
import distutils
from pkg_resources import parse_requirements
+from setuptools.py31compat import TemporaryDirectory
__all__ = ['get_requires_for_build_sdist',
'get_requires_for_build_wheel',
@@ -182,14 +183,17 @@ def build_wheel(self, wheel_directory, config_settings=None,
metadata_directory=None):
config_settings = self._fix_config(config_settings)
wheel_directory = os.path.abspath(wheel_directory)
- sys.argv = sys.argv[:1] + ['bdist_wheel'] + \
- config_settings["--global-option"]
- self.run_setup()
- if wheel_directory != 'dist':
- shutil.rmtree(wheel_directory)
- shutil.copytree('dist', wheel_directory)
-
- return _file_with_extension(wheel_directory, '.whl')
+ with TemporaryDirectory(dir=wheel_directory) as tmp_dist_dir:
+ sys.argv = sys.argv[:1] + [
+ 'bdist_wheel', '--dist-dir', tmp_dist_dir
+ ] + config_settings["--global-option"]
+ self.run_setup()
+ wheel_basename = _file_with_extension(tmp_dist_dir, '.whl')
+ wheel_path = os.path.join(wheel_directory, wheel_basename)
+ if os.path.exists(wheel_path):
+ os.remove(wheel_path)
+ os.rename(os.path.join(tmp_dist_dir, wheel_basename), wheel_path)
+ return wheel_basename
def build_sdist(self, sdist_directory, config_settings=None):
config_settings = self._fix_config(config_settings)
There was a problem hiding this comment.
@shashanksingh28 I have made a few changes and cleaned up the history a bit, but this looks ready to go to me, assuming @benoit-pierre has no objections.
Just for your future edification, the things I changed:
- Some of the lines were very long, so I trimmed them to 80 characters. That's not a hard rule of the project, but >100 characters is pretty much out.
- I reworded the changelog. The changelog is intended to document to end users what has changed between versions. They don't really care about the implementation details like whether a temporary directory is used, they just just care what user-facing behavior was fixed.
- I prefer to have a nice explanation of the bug in the commit message, and to have atomic commits where the test suite should pass on every commit. I squashed together all the fixup commits.
Also, one last thing to note, it would be better if you worked from a branch rather than working in the master branch of your repository. It makes it harder for maintainers to update your branch, and I've seen it cause many other problems before as well. Once this is merged I recommend either deleting your fork and making a new one or doing a hard reset on your master branch against the upstream master branch, then always making PRs against a feature branch.
Thanks so much for your contribution, it is much appreciated!
@benoit-pierre Looks good? Should we merge?
`build_meta.build_wheel` assumes that the only wheel in its output directory is the one it builds, but prior to this, it also used the `dist/` folder as its working output directory. This commit uses a temporary directory instead, preventing an error that was triggered when previously-generated wheel files were still sitting in `dist/`. Fixes GH pypa#1671
|
@benoit-pierre Fixed the tweaks, let me know what you think of the new changelog. Also, for some reason pypy3 is having test failures, not sure if that's related to these changes or not. |
|
I think this is related to #1730, but it's hard to tell because Travis is timing out. I suspect that either PyPy or something we're doing is preventing some files or processes from being closed on PyPy, and that causes problems with this module in particular, because it opens and a ton of files (possibly hundreds?). We should probably try to actually resolve #1730. |
pganssle
changed the title
pganssle
changed the title
|
Thanks @shashanksingh28 and @benoit-pierre and everyone else who contributed to fixing this issue. |
|
Note that |
|
@benoit-pierre Would you mind creating a new issue for |
|
No release since April 22, @pganssle can we do a release from master? |
|
I'm on it. |
|
@jaraco: the upload to PyPI failed. |
|
I did notice that. I need to file a ticket for it. |
|
For which project? I have not tested the PyPI token support yet myself, did not know it existed before I saw your commit. |
|
For this project. I successfully used the token approach already. I'll describe in the ticket. |
Summary of changes
build_meta.build_wheeluses a temporary directory for cases wherewheel_directoryalready hasexisting
.whlfiles (so as to not throw unpacking error).Changes from the patch discussed in the issue:
Closes #1671
Pull Request Checklist